lint: Standardize explicit type coercion to enhance code clarity #3693
Conversation
|
dosubot
bot
added
size:L
KJ7LNW
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
KJ7LNW
force-pushed
the
feat-no-implicit-coercion
branch
2 times, most recently
from
2b91975 to
4bec641
Compare
|
Hey @KJ7LNW, Do you think you can rebase this to properly review it and potentially merge it? Thank you! |
daniel-lxs
added
PR - Changes Requested
and removed
PR - Needs Preliminary Review
labels
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Changes Requested]
in Roo Code Roadmap
I can do that. rebase is pretty simple: instead of rebasing, I delete the commit that apply is the change and have Roo re-apply them because they are not that many changes. |
Add no-implicit-coercion and no-empty rules to improve code quality across the codebase: - no-implicit-coercion: Enforces explicit, context-aware type comparisons to address ambiguity issues with falsy values and improve code clarity - no-empty: Requires all code blocks to have meaningful content, preventing silent error suppression and improving code readability This change includes: - Adding detailed documentation in .roo/rules/eslint-no-implicit-coercion.md with specific guidance on proper type checking patterns - Adding documentation for no-unused-vars rule enforcement in .roo/rules/eslint-no-unused-vars.md - Explicitly adding both rules as "error" in both root and webview-ui configs - Setting --max-warnings=0 in lint scripts to ensure strict enforcement These changes ensure developers use explicit type checks and properly document empty blocks, particularly important for distinguishing between null, undefined, and empty strings in conditional logic. Fixes: #3692 Signed-off-by: Eric Wheeler <[email protected]>
Enforce the `no-useless-catch` ESLint rule across the codebase. This rule disallows `catch` clauses that only rethrow the caught error, which provides no additional value and can make debugging harder. This change adds the rule as an "error" to: - The root ESLint configuration ([](/.eslintrc.json)) - The webview UI ESLint configuration ([](webview-ui/.eslintrc.json)) This ensures that all `catch` blocks either handle the error meaningfully or are omitted if the error should propagate. Signed-off-by: Eric Wheeler <[email protected]>
Replace implicit boolean coercions (git diff --staged | catvariable) with explicit type-aware checks to satisfy the no-implicit-coercion ESLint rule. Also add debug logging to empty blocks to satisfy the no-empty rule. - Use type-specific checks (typeof x === 'string') for string variables - Use explicit comparisons (x === true) for boolean variables - Remove redundant null checks for variables that can't be null - Add console.debug statements to empty catch blocks Signed-off-by: Eric Wheeler <[email protected]>
Refine guidance on leveraging TypeScript types for explicit boolean checks. Clarify that redundant checks for conditions already guaranteed by type definitions (e.g., null checks for `SomeType \| undefined`, or undefined checks for non-optional members) should be omitted. Signed-off-by: Eric Wheeler <[email protected]>
KJ7LNW
force-pushed
the
feat-no-implicit-coercion
branch
from
b3e20d6 to
f824807
Compare
|
tests are currently passing it and there are no merge conflicts because I just cleaned this all up, as well as validated the correctness of each change: Please merge this ASAP to avoid duplicating that effort in the future, because eslint will handle all of these issues going forward. |
daniel-lxs
moved this from PR [Changes Requested]
to PR [Needs Review]
in Roo Code Roadmap
|
Looks good to me, great effort. |
@mrubens "linting" PRs tend to diverge quickly so if you can review and merge this before other pull requests that might conflict, then I would appreciate it! |
mrubens
left a comment
mrubens
left a comment
There was a problem hiding this comment.
In general I want to be really judicious with the linting errors that we add to the project and only add ones that clearly impact the correctness of the code. I don’t want to constrain the LLM output or add to the system prompt unnecessarily. I’m sorry about any conflicts this will add, but I think we should discuss more whether these rules are worth it.
Certainly! I would not have spent the time on this without good reason: In short, failing to differentiate between empty (ie,
This PR was motivated by hitting bugs around Since there are so many developers of varying experience that rely on AI to make changes to Roo, a small amount of Here is one example that I have hit in the past: When I was originally troubleshooting vscode terminal shell integration escape sequence issues, AI would consistently do something like the following, even though I told it numerous times that an "empty" match is valid: // this cannot differentiate between an empty match and no match:
matches = matches(/$prefix(.*)$/)
if (matches[0]) // fails in "" but "" != undefined
success
else
failure If we add these
What do you mean by constrain the output? This does not prevent any particular implementation, it just requires developers to enforce type-safety.
Some instruction to the model specific to Roo-programming guidelines is required so that the model does not simply do change These |
daniel-lxs
moved this from PR [Needs Review]
to PR [Needs Prelim Review]
in Roo Code Roadmap
|
Moving to draft temporarily |
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Draft / In Progress]
in Roo Code Roadmap
hannesrudolph
added
PR - Draft / In Progress
and removed
PR - Needs Preliminary Review
labels
|
stale |
github-project-automation
bot
moved this from PR [Draft / In Progress]
to Done
in Roo Code Roadmap
4 participants
Context
This PR addresses the need to standardize explicit type coercion throughout the codebase to enhance code clarity and prevent ambiguity related to falsy value evaluation.
Implementation
These changes improve code quality by:
How to Test
Get in Touch
Discord: KJ7LNW
Fixes #3692
Important
Enforces explicit type coercion and addresses unused variables across the codebase, enhancing code clarity and consistency.
no-implicit-coercionrule in.eslintrc.jsonandwebview-ui/.eslintrc.json.no-emptyrule to enforce non-empty block statements.eslint-no-implicit-coercion.mdandeslint-no-empty.mdfor guidance on handling these rules.modes.test.ts,subtasks.test.ts,task.test.ts,vscode-lm.ts,ContextProxy.ts,importExport.ts,executeCommandTool.ts,writeToFileTool.ts,ClineProvider.ts,webviewMessageHandler.ts,open-file.ts,ExecaTerminalProcess.ts,ShadowCheckpointService.ts,excludes.ts,combineApiRequests.ts,modes.ts,tts.ts,AutoApproveMenu.tsx,ChatView.tsx,TaskActions.tsx,TaskHeader.tsx,HistoryPreview.tsx,HistoryView.tsx,AutoApproveToggle.tsx,ThinkingBudget.tsx,Anthropic.tsx,Gemini.tsx,OpenAI.tsx,OpenAICompatible.tsx,OpenRouter.tsx,ChatInput.tsx,ChatMessages.tsx,useOpenRouterKeyInfo.ts,useRequestyKeyInfo.ts.lint:extensionscript inpackage.jsonto treat warnings as errors.This description was created by
for 4bec6418b287c477b863054e4fa033ed0762930f. You can customize this summary. It will automatically update as commits are pushed.